Skip to content

Fix fire-and-forget background task in LocalFeatureFlagsLoader - #161

Merged
haacked merged 4 commits into
mainfrom
haacked/fix-fire-and-forget
Feb 26, 2026
Merged

Fix fire-and-forget background task in LocalFeatureFlagsLoader#161
haacked merged 4 commits into
mainfrom
haacked/fix-fire-and-forget

Conversation

@haacked

@haacked haacked commented Feb 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Await the polling task during disposal so in-flight API calls complete before resources are disposed, preventing ObjectDisposedException
  • Fix disposal ordering in PostHogClient.DisposeAsync() so the API client remains alive while the polling task winds down
  • Mark _pollingTask volatile for cross-thread visibility consistency
  • Use GetAwaiter().GetResult() instead of .Wait() in all sync Dispose bridges for cleaner exception stack traces
  • Add tests for graceful disposal during in-flight poll, double-dispose, and disposal before polling starts

Follows the same pattern established in AsyncBatchHandler (PR #158).

Fixes #159

Test plan

  • Existing tests pass on both net8.0 and netcoreapp3.1
  • New CompletesGracefullyDuringInFlightPoll test passes on both frameworks
  • New DoesNotDisposeTwice test passes on both frameworks
  • New CompletesGracefullyWhenPollingNeverStarted test passes on both frameworks

@haacked
Fix fire-and-forget background task in LocalFeatureFlagsLoader
98ff404 
Await the polling task during disposal so in-flight API calls complete
before resources are disposed. Follows the same pattern established in
AsyncBatchHandler (PR #158).

Fixes #159
@github-actions

github-actions Bot commented Feb 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

posthog-dotnet Compliance Report

Date: 2026-02-26 02:42:45 UTC
Duration: 119991ms

✅ All Tests Passed!

29/29 tests passed

Capture Tests

✅ 29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 149ms
Format Validation.Event Has Uuid ✅ 125ms
Format Validation.Event Has Lib Properties ✅ 111ms
Format Validation.Distinct Id Is String ✅ 110ms
Format Validation.Token Is Present ✅ 111ms
Format Validation.Custom Properties Preserved ✅ 115ms
Format Validation.Event Has Timestamp ✅ 110ms
Retry Behavior.Retries On 503 ✅ 8122ms
Retry Behavior.Does Not Retry On 400 ✅ 2021ms
Retry Behavior.Does Not Retry On 401 ✅ 2013ms
Retry Behavior.Respects Retry After Header ✅ 8119ms
Retry Behavior.Implements Backoff ✅ 22133ms
Retry Behavior.Retries On 500 ✅ 6118ms
Retry Behavior.Retries On 502 ✅ 6118ms
Retry Behavior.Retries On 504 ✅ 6119ms
Retry Behavior.Max Retries Respected ✅ 22033ms
Deduplication.Generates Unique Uuids ✅ 116ms
Deduplication.Preserves Uuid On Retry ✅ 6116ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 13125ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 6124ms
Deduplication.No Duplicate Events In Batch ✅ 116ms
Deduplication.Different Events Have Different Uuids ✅ 111ms
Compression.Sends Gzip When Enabled ✅ 107ms
Batch Format.Uses Proper Batch Structure ✅ 107ms
Batch Format.Flush With No Events Sends Nothing ✅ 105ms
Batch Format.Multiple Events Batched Together ✅ 113ms
Error Handling.Does Not Retry On 403 ✅ 2011ms
Error Handling.Does Not Retry On 413 ✅ 2013ms
Error Handling.Retries On 408 ✅ 6117ms

@haacked
Add disposal tests and mark _pollingTask volatile
3f8af57 
Add three tests for LocalFeatureFlagsLoader disposal:
- DoesNotDisposeTwice: concurrent double-dispose completes without exception
- CompletesGracefullyWhenPollingNeverStarted: dispose before any polling
- Await disposeTask after timeout check to surface disposal exceptions

Mark _pollingTask volatile for consistency with other cross-thread fields
and to make the intent explicit, since unlike AsyncBatchHandler's readonly
task fields, this one is lazily assigned after construction.
@haacked
haacked requested a review from Copilot February 26, 2026 01:52
Copilot AI reviewed Feb 26, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a shutdown race in local feature-flag polling by tracking and awaiting the background polling task during disposal, and by ensuring PostHogClient.DisposeAsync() keeps the API client alive until background work winds down.

Changes:

  • Track the local feature-flag polling Task and await it during LocalFeatureFlagsLoader.DisposeAsync().
  • Reorder PostHogClient.DisposeAsync() to dispose background components before disposing the API client.
  • Add a regression test to verify disposal completes while a poll is in-flight.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/PostHog/Features/LocalFeatureFlagsLoader.cs Stores the polling task and implements async disposal to cancel/await polling before disposing timer/CTS.
src/PostHog/PostHogClient.cs Disposes background components before disposing _apiClient, avoiding shutdown-time ObjectDisposedException.
tests/UnitTests/Features/LocalFeatureFlagsLoaderTests.cs Adds a test that blocks an in-flight poll and asserts DisposeAsync completes cleanly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/PostHog/Features/LocalFeatureFlagsLoader.cs Outdated
Comment thread src/PostHog/Features/LocalFeatureFlagsLoader.cs Outdated
Comment thread src/PostHog/Features/LocalFeatureFlagsLoader.cs Outdated
Comment thread src/PostHog/PostHogClient.cs Outdated
@haacked
Use GetAwaiter().GetResult() instead of .Wait() in sync Dispose
4085324 
Avoids wrapping exceptions in AggregateException, giving clearer
stack traces from disposal paths. Applied consistently across all
three sync-over-async Dispose bridges.

Also fix misleading comment in LocalFeatureFlagsLoader — cancellation
terminates the in-flight request, it doesn't wait for it to complete.
@haacked
haacked marked this pull request as ready for review February 26, 2026 02:37
@haacked
haacked requested a review from a team February 26, 2026 02:37
@haacked
Wrap async disposal in try/finally to guarantee resource cleanup
855ce2a 
Ensures timers, CTS, and API clients are always disposed even if
awaiting a background task throws. The exception still propagates —
this just prevents resource leaks on top of bugs.

Applied consistently to LocalFeatureFlagsLoader, AsyncBatchHandler,
and PostHogClient.
greptile-apps[bot]
greptile-apps Bot reviewed Feb 26, 2026
View reviewed changes

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@haacked
haacked merged commit 2bac535 into main Feb 26, 2026
12 checks passed
@haacked
haacked deleted the haacked/fix-fire-and-forget branch February 26, 2026 16:44
@haacked haacked mentioned this pull request Mar 9, 2026
Closed
@haacked haacked mentioned this pull request Mar 23, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
Copilot code review Copilot Copilot left review comments
@marandaneto marandaneto marandaneto approved these changes

Assignees

No one assigned

Labels

bump-patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LocalFeatureFlagsLoader has fire-and-forget background task pattern

3 participants

@haacked @marandaneto